Add orchestration execution tracing #441

philliphoff · 2025-06-12T23:17:48Z

NOTE: !!! Do not merge until the associated protobuf PR is committed and this PR has been updated to match. !!!

Ports Durable Task Framework (DTFx) tracing behavior to the Durable Task .NET SDK gRPC layer to allow end-to-end tracing of orchestrations from creation to completion, including:

Client-side
- Creating orchestrations
- Raising events
Worker-side
- Executing orchestrations (where all activity is parented to a single "logical" span)
- Creating suborchestrations
- Creating timers
- Sending events

With this change, users can get a complete picture of orchestration execution including how their own traces dovetail with Durable Task execution, as shown in the following example, which follows creation of an orchestration starting from a web API, to execution by a DT worker, with suborchestrations, and back to the web API as part of task execution.

This change depends on corresponding changes to the Durable Task protobuf definitions. It also assumes any backend properly accepts and returns the parent trace contexts given it by the SDK.

Resolves #439

sophiatev

Just out of curiosity: do we plan to do the same thing for entities at some point?

Directory.Packages.props

src/Grpc/orchestrator_service.proto

src/Shared/Grpc/Tracing/TraceHelper.cs

src/Shared/Grpc/ProtoUtils.cs

sophiatev · 2025-07-16T20:02:09Z

src/Worker/Grpc/GrpcOrchestrationRunner.cs

            completionToken: string.Empty, /* doesn't apply */
-            entityConversionState: null);
+            entityConversionState: null,
+            // TODO: Should this activity be created?


This might lead to redundancy in the case that this is being used by Durable Functions (since DT.Core will also create the trace activity in that case, I think)

I was primarily targeting these changes at DT used separately from DF. I will see if I can test the two together. I would expect that DF uses a differently-named activity source, though, so overlap should be ok. I wouldn't want to not trace something in DT because it's being traced in DF.

test/Grpc.IntegrationTests/GrpcSidecar/Dispatcher/GrpcSubOrchestrationInstanceCreatedEvent.cs

test/Grpc.IntegrationTests/GrpcSidecar/Dispatcher/TaskOrchestrationDispatcher.cs

src/Shared/Grpc/ProtoUtils.cs

sophiatev · 2025-07-16T20:51:28Z

src/Shared/Grpc/ProtoUtils.cs

                        InstanceId = subOrchestrationAction.InstanceId,
                        Name = subOrchestrationAction.Name,
                        Version = subOrchestrationAction.Version,
+                        ParentTraceContext = clientActivityContext is not null


Rather than having to creating a whole TraceContext object and having to create these strings manually, can you just save the span ID? Since it looks like that is the only information TraceHelper.CreateTraceActivityForSchedulingSubOrchestration and TraceHelper.StartTraceActivityForSchedulingTask actually use

Unless I misunderstand, while it's true that TraceHelper only use a subset of TraceContext, the protos for those history events themselves offer a parent TraceContext so this is necessary to satisfy that original contract (e.g. across all platforms). Currently, for example, DTS is passing the original parent context at orchestration creation to those events, which is be better than nothing, but results in gaps in the trace. Allowing for the context to round trip gives the SDKs flexibility in adding layers to the trace.

That said, I did do some cleanup to simplify/consolidate creation of these context objects.

Yeah I see what you mean, it just makes me quite nervous that we're having to build these manually, and that we're altering the parent trace context to now hold an injected client span id rather than the parent trace activity's span id. An alternative approach is to add an additional field to the proto object corresponding to the clientSpanId, and just set the TraceParent as Activity.Id as you normally would. If anything, I'd say that this is truer to the original contract: other SDKs accessing this object would expect it hold the span id of the parent trace Activity, not an injected client span id.

All that being said, I do see that DT.Core builds a traceparent manually in an identical way so maybe it's not a cause for concern

That's where I got it from, so I figured it ought to work at least as well here and I was trying to minimize the changes needed to the protos. That said, I'm also not crazy about having to build the values by hand.

philliphoff · 2025-07-18T22:01:19Z

Just out of curiosity: do we plan to do the same thing for entities at some point?

My first step was just to reach parity with DTFx but I think we'd definitely want to extended to entities/schedules.

cgillum

A few nit-picks as I go through the PR. I have not yet reviewed everything.

cgillum · 2025-07-25T18:53:01Z

src/Shared/Grpc/Tracing/TraceHelper.cs

+            return null;
+        }
+
+        if (startEvent.ParentTraceContext is null || !ActivityContext.TryParse(startEvent.ParentTraceContext.TraceParent, startEvent.ParentTraceContext.TraceState, out ActivityContext activityContext))


nit: can you wrap these long lines? Per the .editorconfig file (which not all IDEs support natively, though VSCode.dev seems to), we try to keep line length to a maximum of 120 characters.

I believe I've gotten all of these.

cgillum · 2025-07-25T18:53:50Z

src/Shared/Grpc/Tracing/TraceHelper.cs

+/// <remarks>
+/// Adapted from "https://github.com/Azure/durabletask/blob/main/src/DurableTask.Core/Tracing/TraceHelper.cs".
+/// </remarks>
+class TraceHelper


nit: make this static class.

cgillum · 2025-07-25T18:54:04Z

src/Shared/Grpc/Tracing/TraceActivityConstants.cs

+/// <summary>
+/// Constants for trace activity names used in Durable Task Framework.
+/// </summary>
+class TraceActivityConstants


nit: make this static class

cgillum · 2025-07-25T18:56:20Z

src/Shared/Grpc/Tracing/TraceHelper.cs

+            }
+        }
+
+        if (Activity.Current is not null)


nit: rather than checking separately for Activity.Current being null, could we just move this logic into the previous newActivity != null code block?

Yes, this makes sense.

cgillum · 2025-07-25T18:58:32Z

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs

+            {
+                P.HistoryEvent? GetSuborchestrationInstanceCreatedEvent(int eventId)
+                {
+                    var subOrchestrationEvent =


nit: per the existing conventions in this repo, please try to use explicit type names instead of var. It makes the code easier to understand.

I believe I've updated all of these.

philliphoff · 2025-08-07T17:58:34Z

@sophiatev Is there more feedback on this change?

sophiatev

Looks good to me, but just a few questions:

How did we actually get this working? When I was trying to do add distributed tracing for entities I had to do a lot of work-arounds and add most of it to DT.Core because we seemingly couldn't add an Activity source to the dotnet SDK at the time. Did that change? (I wish I would have done my work after you did then :D. It would have improved my implementation by quite a bit)
Does the user have to enable distributed tracing in their settings somehow to get this tracing?
Do we want to make an Activity in the GrpcOrchestrationRunner after all?

philliphoff · 2025-08-08T16:44:09Z

How did we actually get this working? When I was trying to do add distributed tracing for entities I had to do a lot of work-arounds and add most of it to DT.Core because we seemingly couldn't add an Activity source to the dotnet SDK at the time. Did that change?

@sophiatev I got it working/tested by creating an app that references a private build of the SDK and enables this new activity source (Microsoft.DurableTask), and then a backend that respects the protobuf changes that allow the context to be properly passed (changes coming to DTS once this goes in). I'm not sure what issues you ran into before, but AFAIK, any 3rd party can add activity sources.

Does the user have to enable distributed tracing in their settings somehow to get this tracing?

Yes, users collecting traces would have to add Microsoft.DurableTask as a source.

Do we want to make an Activity in the GrpcOrchestrationRunner after all?

Are you referring to the earlier comment about Durable Functions also having an activity for that method, or something else? If the former, I'd say yes; the DF traces are from a different source so we'd want to ensure it's logged when DT is used on its own.

@sophiatev With tentative approval here, we can now commit the corresponding protobuf changes. That will then mean a small update to this PR to bring them across the "proper way", which will need a re-approval.

sophiatev · 2025-08-08T21:20:46Z

Yes, users collecting traces would have to add Microsoft.DurableTask as a source.

Perhaps a silly question, but have you tested without doing this to make sure that nothing breaks? I made the mistake of not doing so (in my case, not trying a run without distributed tracing enabled) which ended up in a null exception for customers. Want to make sure other's don't repeat my mistakes :D

Are you referring to the earlier comment about Durable Functions also having an activity for that method, or something else? If the former, I'd say yes; the DF traces are from a different source so we'd want to ensure it's logged when DT is used on its own.

Yes, I am. But it looks like the GrpcOrchestrationRunner is still not creating an Activity? Doesn't this imply that it should? (Although I'm not sure this is called when DT is used on its own, maybe there's a scenario where it will be in the future?)

philliphoff · 2025-08-11T17:37:28Z

Perhaps a silly question, but have you tested without doing this to make sure that nothing breaks? I made the mistake of not doing so (in my case, not trying a run without distributed tracing enabled) which ended up in a null exception for customers. Want to make sure other's don't repeat my mistakes :D

@sophiatev That's a good test case; yes, I've verified that execution proceeds when tracing has not been enabled.

Yes, I am. But it looks like the GrpcOrchestrationRunner is still not creating an Activity? Doesn't this imply that it should? (Although I'm not sure this is called when DT is used on its own, maybe there's a scenario where it will be in the future?)

I verified that GrpcOrchestrationRunner (and GrpcEntityRunner) are not called within the SDK and only called by the DF extension (and no other external libraries as far as GH can find). At least for now, I'm inclined to leave tracing out of them, given the specific focus of this PR on DT, and it'd be a different source from whatever AF/DF was doing anyway, plus there doesn't seem to be an equivalent for activities so there would still be gaps in the DT traces? If and when it is needed to provide a coherent tracing story for DF, I'd be open to adding them, but I feel like that's a completely different set of tests and fixes.

philliphoff · 2025-08-14T22:17:04Z

@sophiatev Is there a regular schedule/process for new releases?

philliphoff added 29 commits May 14, 2025 21:33

WIP of PoC of tracing.

5f36cd4

Wiring tracing metadata through test server.

76fdee2

Ensure orchestration traces are parented correctly.

4681dc0

Check orchestration activities are same logical activity.

3421304

Test activity traces.

b951b5d

Assert tags of orchestration activities.

06697bb

Sketch emitting client activity trace.

b8bd6ca

Exclude Rider files.

47e2018

Attempt to capture activity/orchestration failures.

588d518

Stop orchestration trace when execution completes.

59d99b2

Update protos to match backend.

e431238

Add/set partent trace context to suborchestration action.

4d2c581

Port suborchestration tracing.

5b35016

Add tracing for events sent by worker.

2a80911

Add tracing for client-sent events.

889640e

Merge branch 'main' into philliphoff-orchestration-tracing

183c4a1

Add tracing for orchestration timers.

bc42ee6

Sketch basic tracing test.

5b44cef

Test activity types.

fa62a5f

Add test for sent events.

4ca345d

Add test for timer event.

11803e7

Add test for client raised events.

19cf60f

Move files to tracing namespace.

a37b364

Clean up files.

7e8262c

Fixup test for suborchestration tracing.

6b67b9d

Add test for suborchestration failure.

73c8ee0

Revert change.

9497ad5

Merge branch 'main' into philliphoff-orchestration-tracing

99aea1b

Eliminate the unnecessary activity ID (as it can be derived).

a51a235

philliphoff requested a review from cgillum July 14, 2025 19:32

cgillum requested review from bachuv and sophiatev July 14, 2025 19:43

sophiatev reviewed Jul 16, 2025

View reviewed changes

philliphoff added 2 commits July 18, 2025 13:31

Fix up a number of warnings.

14afe63

Fixup doc and style warnings.

41c44f1

philliphoff added 4 commits July 18, 2025 15:47

Revert package props.

f9f8ea3

Updates per PR feedback.

0d65131

Add orchestration status tag on completion.

1afa837

Update for move of orchestration trace context to "outer" message.

5a5c199

cgillum reviewed Jul 25, 2025

View reviewed changes

philliphoff and others added 3 commits July 31, 2025 11:23

Merge branch 'main' into philliphoff-orchestration-tracing

6288cea

Fix test breaks.

1dc3277

Updates per more PR feedback.

ae9a2fd

philliphoff requested a review from sophiatev August 7, 2025 17:58

Merge branch 'main' into philliphoff-orchestration-tracing

bcf6ecf

sophiatev approved these changes Aug 7, 2025

View reviewed changes

Update protobuf version.

6254ca4

sophiatev approved these changes Aug 11, 2025

View reviewed changes

philliphoff merged commit b3064cb into microsoft:main Aug 12, 2025
4 checks passed

philliphoff deleted the philliphoff-orchestration-tracing branch August 12, 2025 16:25

Add orchestration execution tracing #441

Add orchestration execution tracing #441

Uh oh!

Conversation

philliphoff commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sophiatev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophiatev Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philliphoff commented Jul 18, 2025

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philliphoff commented Aug 7, 2025

Uh oh!

sophiatev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philliphoff commented Aug 8, 2025

Uh oh!

sophiatev commented Aug 8, 2025

Uh oh!

philliphoff commented Aug 11, 2025

Uh oh!

Uh oh!

philliphoff commented Aug 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

philliphoff commented Jun 12, 2025 •

edited

Loading

sophiatev Jul 21, 2025 •

edited

Loading

sophiatev left a comment •

edited

Loading